Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - Optimize Events::extend and impl std::iter::Extend #2207

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Optimize Events::extend and impl std::iter::Extend #2207

wants to merge 3 commits into from
+41 −10

Conversation

NathanSWard
Copy link
Contributor

The previous implementation of Events::extend iterated through each event and manually sent it via Events:;send.
However, this could be a minor performance hit since calling Vec::push in a loop is not optimal.
This refactors the code to use Vec::extend.

@NathanSWard NathanSWard added the A-ECS Entities, components, systems, and events label May 18, 2021
@alice-i-cecile alice-i-cecile added the C-Performance A change motivated by improving speed, memory usage or compile times label May 18, 2021
@mockersf
Copy link
Member

Wouldn't that make the (more common) case of sending only one event worse?

@NathanSWard
Copy link
Contributor Author

Wouldn't that make the (more common) case of sending only one event worse?

It shouldn't no.
Since std::iter::once is an ExactSizedIterator it should have the exact same performance as push().
However, I'm working on making benchmarks and assembly comparisons for the two.

Arguably, if there is a regression for the single send case, then have two different implementation is probably the best way to go.

@NathanSWard NathanSWard changed the title Implement Events::extend in terms of Vec::extend Optimize Events::extend and impl std::iter::Extend May 18, 2021
@NathanSWard NathanSWard force-pushed the nward/events-refactor branch 2 times, most recently from b30cdd0 to 8b590bc Compare May 18, 2021 22:57
@NathanSWard
Copy link
Contributor Author

@mockersf
I ended up just side stepping the problem and now utilizing std::iter::Extend for the impl.

@mockersf
Copy link
Member

yay for implementing std traits!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 18, 2021
@cart
Copy link
Member

cart commented May 19, 2021

Looks good to me! I agree that separate extend and send impls makes sense here. The code is simple enough that I'd rather just inline it.

@cart
Copy link
Member

cart commented May 19, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 19, 2021
@NathanSWard
Optimize Events::extend and impl std::iter::Extend (#2207)
a42343d 
The previous implementation of `Events::extend` iterated through each event and manually `sent` it via `Events:;send`.
However, this could be a minor performance hit since calling `Vec::push` in a loop is not optimal.
This refactors the code to use `Vec::extend`.
@bors
Copy link
Contributor

bors bot commented May 19, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Optimize Events::extend and impl std::iter::Extend [Merged by Bors] - Optimize Events::extend and impl std::iter::Extend May 19, 2021
@bors bors bot closed this May 19, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@NathanSWard @ostwilkens
Optimize Events::extend and impl std::iter::Extend (bevyengine#2207)
72a6a14 
The previous implementation of `Events::extend` iterated through each event and manually `sent` it via `Events:;send`.
However, this could be a minor performance hit since calling `Vec::push` in a loop is not optimal.
This refactors the code to use `Vec::extend`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NathanSWard @mockersf @cart @alice-i-cecile